Repository navigation
fix(pptx): convert soft line breaks instead of emitting a vertical tab - #2513
Lukas (L4XB) wants to merge 2 commits into
Conversation
Shift+Enter inside a PowerPoint paragraph is an `<a:br/>`, and python-pptx reports it as `\v`. The converter passes `shape.text`, `cell.text`, the notes text and the chart title straight through, so that vertical tab reaches the Markdown. It is a C0 control, not Markdown: renderers drop it, so the two lines run together, and nothing downstream treats it as whitespace either. Normalize it where the text enters. A shape's own paragraphs already arrive joined with newlines, so a soft break reads the same way. A table cell keeps its row: the newline goes into the HTML the table is built from, where it is ordinary whitespace and collapses to a space. A slide title is a special case: an ATX heading cannot span lines, so a newline there would close the heading and leave the rest as body text. Titles now collapse their line breaks to spaces, which also fixes the multi-paragraph titles that already produced a stray body line.
…eaks main consolidated the tests by format (microsoft#2579), so this PR's test moves from test_module_misc.py to test_pptx.py, before test_pptx_chart_multi_series_conversion where it stood, and uses that file's module-level pptx imports.
XU (kokokoXUY)
left a comment
XU (kokokoXUY)
left a comment
There was a problem hiding this comment.
The fix is right and scoped correctly; one heading case needs the same treatment
Short version: I reproduced the reported problem on the base commit and confirmed that
the patch removes the vertical tab on every path it touches, with no change to the
existing PPTX reference vectors. I would merge it, but I would first apply the same
"a heading is one line" rule to the chart title and add a test for that path. Evidence
below.
What the patch does
- Adds
_normalize_soft_line_breaks(text) -> str
(packages/markitdown/src/markitdown/converters/_pptx_converter.py:34-43), a
text.replace("\v", "\n"), and applies it at four places: shape text (:120), notes
text (:158-162), each table cell beforehtml.escape(:330) and the chart title
(:352). - The slide title additionally collapses newlines to spaces (
:125), replacing the old
text.lstrip()withtext.replace("\n", " ").strip(). - New test
test_pptx_soft_line_breaks_are_not_vertical_tabs
(packages/markitdown/tests/test_pptx.py, in the conversion-regressions section),
which builds a deck with a soft break in a title, a text box, a table header cell, a
table body cell and the notes.
What I verified locally
Trees: base 4cc9fa1 and head d253f5d, as two detached worktrees of a local clone.
python-pptx 1.0.2. Every conversion went through the
public API, MarkItDown().convert(path), with PYTHONPATH=<tree>\packages\markitdown\src
so that the tree under test is the one imported (the probe prints
markitdown.__file__ to prove it).
- The PR's own scenario (title, text box, 2x2 table, notes, each with a soft break):
base output contains five vertical tabs
('\n# TITLE ONE\x0bTITLE TWO\nADDRESS LINE ONE\x0bADDRESS LINE TWO\n| HEADER A\x0bHEADER B | H2 |\n| --- | --- |\n| CELL LINE ONE\x0bCELL LINE TWO | C2 |\n\n### Notes:\nNOTE LINE ONE\x0bNOTE LINE TWO');
head output contains none and reads# TITLE ONE TITLE TWO,
ADDRESS LINE ONE+ newline +ADDRESS LINE TWO,
| HEADER A HEADER B | H2 |,| CELL LINE ONE CELL LINE TWO | C2 |, and
NOTE LINE ONE+ newline +NOTE LINE TWO. The table row really does stay one row,
as the description claims. - Multi-paragraph title (the second case the description mentions): base emits
# FIRST PARA+ newline +SECOND PARA, so the heading ends early; head emits
# FIRST PARA SECOND PARA. - A soft break in a body placeholder, in a text box inside a group shape, two
consecutive breaks (A\v\vB->Ablank lineB), a leading break and a trailing
break all normalize as intended; a title with trailing spaces and a title ending in a
break are unchanged from base. - Table cell paths:
A\vB->A B; a cell whose text is only a break becomes empty;
header and body cells behave the same way. - Chart title: base
### Chart: CHART TITLE ONE\x0bCHART TITLE TWO; head
### Chart: CHART TITLE ONE+ newline +CHART TITLE TWO. See the first item under
"What I would change". - The new test is a real regression test: run against the base converter it fails
(1 failed, 49 deselected in 1.40s,
packages\markitdown\tests\test_pptx.py:804: AssertionError, showing base output
# TITLE ONE+ newline +TITLE TWO). packages/markitdown/tests/test_pptx.py: 49 passed on base, 50 passed on head.- The six
.pptxreference vectors underpackages/markitdown/tests/test_files/
(includingtest.pptx,pptx_empty_body_run.pptx,pptx_empty_title_run.pptx,
pptx_notes_empty_run.pptx,pptx_notes_missing_body_frame.pptx,
test_svg_no_fallback.pptx) produce byte-identical Markdown on both trees; none of
them contains a vertical tab, so no existing expectation depended on the old output. - python-pptx's
\vis genuinely the outlier here: python-docx already maps
<w:br/>(text wrapping) and<w:cr/>to\n(docx/oxml/text/run.py:182-206in
python-docx 1.2.0), so putting the fix in the PPTX converter, rather than normalizing
document text centrally, matches how the other formats arrive.
What I would change before merging
-
Chart title. After the patch the chart title is normalized but not collapsed, so a
soft break inside it terminates the ATX heading:### Chart: CHART TITLE ONE CHART TITLE TWOThe second line is no longer part of the
### Chart: ...line, which is exactly the
argument the patch uses for the slide title ("an ATX heading cannot span lines").
The old output (### Chart: CHART TITLE ONE\x0bCHART TITLE TWO) kept it on one ugly
line; the new output splits it. Atext.replace("\n", " ")on that path, matching
the slide title, would remove the inconsistency; if the split is deliberate it
deserves a comment, because nothing in the patch says so. -
Test coverage for the two paths above. The new test covers title, text box, table
cells and notes, but not the chart title (the path with the problem in item 1) and
not the multi-paragraph slide title that the description says it also repairs
(only the\vshape of it is covered). Adding a soft break to a chart title and to
a two-paragraph title would pin both behaviours down.
Notes, not blockers
- What a soft break becomes. In Markdown a bare newline is a soft break: a CommonMark
renderer keeps it in the source but renders it as a space, so a strict renderer still
showsADDRESS LINE ONE ADDRESS LINE TWOrather than two lines. A rendered line break
would need two trailing spaces or<br>, andMarkItDown._convert
(packages/markitdown/src/markitdown/_markitdown.py:682-685) currentlyrstrip()s
every line and collapses runs of blank lines, so hard breaks cannot survive today.
PR #2559 ("preserve Markdown hard line breaks", open) is the change that would allow
them, and its description already notes that it does not overlap this PR. Using\n
is consistent with how pptx paragraphs arrive and is the right choice for text
consumers, but it is worth one sentence in the PR description (or the docs) recording
that the result is a soft break, not a rendered<br>. - Out of scope, but visible on the table path this patch touches: a literal
|in a
cell is not escaped, so a cell readingPIPE | INSIDEproduces an extra column
(| | PIPE | INSIDE |); and a cell whose text is<tag>&is emitted as
<tag>&, so the angle-bracket text goes out unescaped and the literal text
&stays an entity that renders as&. Both predate this patch (they are
reproducible on the base commit) and neither is made worse by it.
How this was produced
Scripts: _r234_md2513_probe.py (builds nine fixtures with
python-pptx and converts each one, printing the raw repr), _r234_md2513_vectors.py
(converts every .pptx reference vector). Outputs: _r234_md2513_probe_base.txt,
_r234_md2513_probe_head.txt, _r234_md2513_vectors_base.txt,
_r234_md2513_vectors_head.txt. Fixtures live in a scratch directory beside them.
The red-test run copied the head test file into the base tree, ran it, and restored the
tree (the base worktree is clean again).
Verdict
The core change is correct, minimal and verified end to end at d253f5d; I would merge
it after collapsing the chart-title line the same way as the slide title and adding the
two missing test cases.
2 participants
Summary
Shift+Enter inside a PowerPoint paragraph is an
<a:br/>— one paragraph, twolines — and python-pptx reports it as
\v.PptxConverterpassesshape.text,cell.text, the notes text and the chart title straight through, so thatvertical tab lands in the Markdown:
A vertical tab is a C0 control, not Markdown. A renderer drops it, so the two
lines run together (
ADDRESS LINE ONEADDRESS LINE TWO), and nothing downstreamtreats it as whitespace either — a splitter sees one long line and a tokenizer
sees one strange token. Address blocks, signature blocks and "name / title"
boxes are where this shows up, which is most of the text in a deck's smaller
shapes.
After:
What changed
One helper, applied where the text enters:
\vbecomes\n. A shape's ownparagraphs already arrive joined with
\n, so a soft break now reads the sameway, and the table path needs nothing special — the newline goes into the HTML
the table is built from, where it is ordinary whitespace and collapses to a
space, so the row stays one row.
A slide title is the one place a newline cannot go: an ATX heading ends at the
line break, which would leave the rest of the title as body text. Titles
therefore collapse their line breaks to spaces. That also repairs the
already-broken case of a multi-paragraph title, which produces
# First\nSecondtoday.Tests
test_pptx_soft_line_breaks_are_not_vertical_tabsinpackages/markitdown/tests/test_pptx.py(the PowerPoint suite since #2579) builds a deck with a soft breakin a title, a text box, a header cell, a body cell and the speaker notes, then
asserts no
\vsurvives and each one reads correctly. It fails againstmain's converter.The suite includes the reference
test.pptxvector, so the title change is coveredagainst a real deck as well as the synthetic one.
black==23.7.0(the pinned pre-commit version) leaves both files unchanged.